Skip to content

fix(browser): wait for iframe tester readiness before preparing#10497

Merged
sheremet-va merged 1 commit into
vitest-dev:mainfrom
soconnor-seeq:fix/browser-iframe-ready
Jun 7, 2026
Merged

fix(browser): wait for iframe tester readiness before preparing#10497
sheremet-va merged 1 commit into
vitest-dev:mainfrom
soconnor-seeq:fix/browser-iframe-ready

Conversation

@soconnor-seeq

@soconnor-seeq soconnor-seeq commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

During the investigation for vitest-randomly-hanging-in-CI I (with the help of Codex) initially diagnosed a race condition that was ultimately fixed but I still was getting CI failures / hangs (even after that first fix) — exposing another race condition.

It's a browser-runner readiness race where the orchestrator could send the initial prepare event before the tester <iframe> was actually ready to receive channel messages.

Resolves (do you need an issue for a bug fix?)

This PR adds an explicit tester-ready handshake between the browser tester and orchestrator.

Changes

  • Add a new <iframe> channel event: ready
  • Have the tester post ready only after its channel listener is registered
  • Make the orchestrator wait for the current <iframe> to report ready before sending prepare

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.
  • Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

AI assisted: Codex

soconnor-seeq
soconnor-seeq commented Jun 1, 2026
View reviewed changes
Comment thread packages/browser/src/client/orchestrator.ts Outdated
@netlify

netlify Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 5ba3c64
🔍 Latest deploy log https://app.netlify.com/projects/vitest-dev/deploys/6a21ca7d03a472000851be9f
😎 Deploy Preview https://deploy-preview-10497--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

sheremet-va
sheremet-va requested changes Jun 2, 2026
View reviewed changes
Comment thread packages/browser/src/client/orchestrator.ts Outdated
@sheremet-va

sheremet-va commented Jun 2, 2026

Copy link
Copy Markdown
Member

This PR adds an explicit tester-ready handshake between the browser tester and orchestrator, and tracks <iframe> instance generations so stale ready events from an older <iframe> cannot satisfy readiness for a newly recreated <iframe>.

I am confused why we need multiple id trackers. There can only be one iframe on the page at a time, the iframeId is the name of the test file. Iframes are always removed when the test is finished even in watch mode, we need to recreate them.

There should never be any "stale" or "reloaded" iframes

sheremet-va
sheremet-va reviewed Jun 2, 2026
View reviewed changes
Comment thread packages/browser/src/client/orchestrator.ts Outdated
sheremet-va
sheremet-va reviewed Jun 2, 2026
View reviewed changes
Comment thread test/unit/test/iframe-registry.test.ts Outdated
@@ -0,0 +1,150 @@
// @vitest-environment happy-dom

@sheremet-va sheremet-va Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer tests that actually fail without you change (even if not always). Having unit tests for the registry doesn't give me any confidence that API works in a real orchestrator. Especially because there is supposed to only be a single iframe anyway. The idea of having a registry with this premise sounds a bit ridiculous, not gonna lie

@soconnor-seeq soconnor-seeq Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of having a registry with this premise sounds a bit ridiculous, not gonna lie

Yea it was. I was trusting Codex's answers about how it was possible that there would be stale events coming from <iframe>s that needed to be disambiguated.

I've removed this unit test and with Codex was able to make a test/browser/specs/readiness.test.ts integration test file which fails on the current main branch but passes w/ this code change.

@sheremet-va

sheremet-va commented Jun 2, 2026

Copy link
Copy Markdown
Member

I've also noticed that after your previous PR we are getting Test timed out in 360000ms errors more often in brower tests

@soconnor-seeq

soconnor-seeq commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

I've also noticed that after your previous PR we are getting Test timed out in 360000ms errors more often in brower tests

Oh dang! Probably best to revert that PR then. 😞

@soconnor-seeq soconnor-seeq force-pushed the fix/browser-iframe-ready branch from d9f85f2 to 34a49e6 Compare June 2, 2026 22:16
soconnor-seeq
soconnor-seeq commented Jun 3, 2026
View reviewed changes
Comment thread packages/browser/src/client/orchestrator.ts
startTime,
otelCarrier: this.traces.getContextCarrier(otelContext),
}).then(resolve, error => reject(this.dispatchIframeError(error)))
this.waitForReady(iframeId)

@soconnor-seeq soconnor-seeq Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the actual bug-fix is. Instead of just assuming we can this.sendEventToIframe({ event: 'prepare', ... }) we now wait until the <iframe> has told us it is ready.

@soconnor-seeq soconnor-seeq force-pushed the fix/browser-iframe-ready branch from 34a49e6 to 62244f4 Compare June 3, 2026 15:07
sheremet-va
sheremet-va requested changes Jun 3, 2026
View reviewed changes
Comment thread packages/browser/src/client/orchestrator.ts Outdated
@soconnor-seeq soconnor-seeq force-pushed the fix/browser-iframe-ready branch from 62244f4 to 5ba3c64 Compare June 4, 2026 18:56
@sheremet-va

sheremet-va commented Jun 4, 2026

Copy link
Copy Markdown
Member

LGTM, please confirm that the fix works for your use case 🙏

@pkg-pr-new

pkg-pr-new Bot commented Jun 4, 2026

Copy link
Copy Markdown
@vitest/browser

npm i https://pkg.pr.new/@vitest/browser@10497

@vitest/browser-playwright

npm i https://pkg.pr.new/@vitest/browser-playwright@10497

@vitest/browser-preview

npm i https://pkg.pr.new/@vitest/browser-preview@10497

@vitest/browser-webdriverio

npm i https://pkg.pr.new/@vitest/browser-webdriverio@10497

@vitest/coverage-istanbul

npm i https://pkg.pr.new/@vitest/coverage-istanbul@10497

@vitest/coverage-v8

npm i https://pkg.pr.new/@vitest/coverage-v8@10497

@vitest/expect

npm i https://pkg.pr.new/@vitest/expect@10497

@vitest/mocker

npm i https://pkg.pr.new/@vitest/mocker@10497

@vitest/pretty-format

npm i https://pkg.pr.new/@vitest/pretty-format@10497

@vitest/snapshot

npm i https://pkg.pr.new/@vitest/snapshot@10497

@vitest/spy

npm i https://pkg.pr.new/@vitest/spy@10497

@vitest/ui

npm i https://pkg.pr.new/@vitest/ui@10497

@vitest/utils

npm i https://pkg.pr.new/@vitest/utils@10497

vitest

npm i https://pkg.pr.new/vitest@10497

@vitest/web-worker

npm i https://pkg.pr.new/@vitest/web-worker@10497

commit: 5ba3c64

@soconnor-seeq

soconnor-seeq commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

LGTM, please confirm that the fix works for your use case 🙏

Yes I tested this PR's build version of vitest in my CI pipeline and w/ it's changes the CI builds pass 20/20. Before they were hanging ~25% of the time.

@sheremet-va sheremet-va merged commit f26552c into vitest-dev:main Jun 7, 2026
20 of 23 checks passed
@sheremet-va sheremet-va mentioned this pull request Jun 9, 2026
Merged
sheremet-va added a commit that referenced this pull request Jun 10, 2026
@sheremet-va @soconnor-seeq
fix(browser): wait for iframe tester readiness before preparing (#10497…
fbc626c 
…) [backport to v4] (#10556)

Co-authored-by: Séamus O'Connor <seamus.oconnor@seeq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sheremet-va sheremet-va sheremet-va approved these changes

Assignees

No one assigned

Labels

cr-tracked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@soconnor-seeq @sheremet-va